Refactor terminal interaction and enhance output handling - Change shell command from 'sh' to 'node' in dev.spec.json for better compatibility. - Implement waitForOutputSettle function to ensure terminal output stabilizes before proceeding. - Add unit tests for typeKeys output settling behavior and interaction with Node REPL. - Validate that typed input is captured correctly in interactive terminal applications.#416
Conversation
- Refactor existing code for better readability - Optimize performance in critical sections - Update documentation to reflect recent changes - Remove deprecated methods and clean up unused imports
- Change shell command from 'sh' to 'node' in dev.spec.json for better compatibility. - Implement waitForOutputSettle function to ensure terminal output stabilizes before proceeding. - Add unit tests for typeKeys output settling behavior and interaction with Node REPL. - Validate that typed input is captured correctly in interactive terminal applications.
WalkthroughIntroduces a comprehensive scope-based process management system enabling named, long-running terminal scopes within the test framework. This includes PTY-backed terminal creation via node-pty, scope registry for lifecycle management, condition-based stdout/stderr waiting, and integration with existing test actions (runShell, typeKeys, terminateScope). Changes
Sequence DiagramsequenceDiagram
participant Test as Test Runner
participant Registry as ScopeRegistry
participant Terminal as Terminal PTY
participant Scope as Named Scope
participant Output as Condition Checker
Test->>Registry: create scope with name
Registry->>Terminal: spawn PTY process
Terminal->>Scope: initialize with process/stdout/stderr
Registry->>Scope: register in registry
Test->>Terminal: write stdin (typeKeys)
Terminal->>Output: capture stdout/stderr
Output->>Scope: append to buffers
Test->>Output: waitForConditions(scope, pattern)
Output->>Scope: poll stdout/stderr
alt Pattern matches
Output-->>Test: conditions met
else Timeout
Output-->>Test: error with current output
end
Test->>Terminal: terminateScope
Terminal->>Scope: SIGTERM signal
alt Process exits
Terminal->>Registry: cleanup scope
else Timeout
Terminal->>Scope: SIGKILL force
Terminal->>Registry: cleanup scope
end
Registry->>Scope: remove from registry
sequenceDiagram
participant Process as Node Process
participant SignalHandler as Signal Handlers
participant Registry as ScopeRegistry
participant Scope as Running Scope
Note over Process: Application Running
alt SIGINT/SIGTERM received
SignalHandler->>Registry: cleanup()
Registry->>Scope: terminate all processes
end
alt Uncaught Exception
SignalHandler->>Registry: cleanup()
Registry->>Scope: kill processes
SignalHandler->>Process: exit with error
end
alt Normal exit
SignalHandler->>Registry: cleanup()
Registry->>Scope: gracefully close
SignalHandler->>Process: exit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot identify and fix broken tests. |
|
@hawkeyexl I've opened a new pull request, #417, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
📝 Documentation updates detected! Updated existing suggestion: Document scope-based session management features |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/config.js (2)
272-290: Don’t mutateconfig.integrations.openApiwhile iterating it (can skip items).Filtering
config.integrations.openApiinside thefor (const openApiConfig of ...)loop can cause elements to be skipped.Proposed fix
async function loadDescriptions(config) { if (config?.integrations?.openApi) { - for (const openApiConfig of config.integrations.openApi) { - try { - openApiConfig.definition = await loadDescription( - openApiConfig.descriptionPath - ); - } catch (error) { - log( - config, - "error", - `Failed to load OpenAPI description from ${openApiConfig.descriptionPath}: ${error.message}` - ); - // Remove the failed OpenAPI configuration - config.integrations.openApi = config.integrations.openApi.filter( - (item) => item !== openApiConfig - ); - } - } + const next = []; + for (const openApiConfig of config.integrations.openApi) { + try { + openApiConfig.definition = await loadDescription(openApiConfig.descriptionPath); + next.push(openApiConfig); + } catch (error) { + log( + config, + "error", + `Failed to load OpenAPI description from ${openApiConfig.descriptionPath}: ${error.message}` + ); + } + } + config.integrations.openApi = next; } }
251-256: Critical: Parameter mismatch ingetAvailableAppscall + globalcwd+ unprotectedprocess.chdir()breaks config loading.At line 253,
getAvailableApps(config)passesconfigdirectly, but the function at line 305 destructures{ config }, makingconfigundefined inside the function. Additionally,cwdis assigned withoutconst/let(line 307), creating a global variable, andprocess.chdir()(lines 308, 371) is not protected by try/finally, risking the process remaining in the wrong directory if an error occurs.The same function is correctly called at src/tests.js:376 and src/tests.js:1029 with
getAvailableApps({ config }).Proposed fix
- config.environment.apps = await getAvailableApps(config); + config.environment.apps = await getAvailableApps({ config }); // Detect available apps. -async function getAvailableApps({ config }) { +async function getAvailableApps(config) { setAppiumHome(); - cwd = process.cwd(); - process.chdir(path.join(__dirname, "..")); - const apps = []; + const cwd = process.cwd(); + const apps = []; + + try { + process.chdir(path.join(__dirname, "..")); - const installedBrowsers = await browsers.getInstalledBrowsers({ - cacheDir: path.resolve("browser-snapshots"), - }); - const installedAppiumDrivers = await spawnCommand("npx appium driver list"); + const installedBrowsers = await browsers.getInstalledBrowsers({ + cacheDir: path.resolve("browser-snapshots"), + }); + const installedAppiumDrivers = await spawnCommand("npx appium driver list"); // ... - // Return to original working directory after finishing with `BROWSERS` - process.chdir(cwd); + return apps; + } finally { + // Return to original working directory even on error + process.chdir(cwd); + } - - return apps; }
🤖 Fix all issues with AI agents
In @src/scopes/cleanup.js:
- Around line 60-64: The uncaught exception handler uses console.error; change
setupCleanupHandlers to accept an optional config parameter
(setupCleanupHandlers(registry, config)) and have uncaughtHandler (and the other
handlers at 66-70) use the framework logger: if (config) log(config, "error",
"Uncaught exception: " + String(err)) else console.error("Uncaught exception:",
err); ensure the handlers close over the optional config, still call await
cleanup() and process.exit(1), and update any similar places (lines 66-70) to
follow the same conditional logging pattern.
In @src/scopes/registry.js:
- Around line 99-113: The cleanup method currently calls scope.process.kill()
without a signal and pushes Promise.resolve(), so it neither requests graceful
termination nor waits for exit; update the cleanup (method cleanup on the
registry class that iterates this.scopes) to first attempt
scope.process.kill('SIGTERM') inside a try/catch, create and push a Promise that
resolves when the child process emits 'exit' or 'close' (and rejects or times
out after a configurable timeout, e.g. 5s), and if the timeout elapses call
scope.process.kill('SIGKILL') and wait for final exit; ensure you remove event
listeners and handle synchronous kill errors to avoid unhandled exceptions, then
await Promise.all(promises) before clearing this.scopes.
In @src/scopes/waitUntil.js:
- Around line 109-114: The error construction that includes raw scope.stdout
(the reject(new Error(...)) in waitUntil.js) can leak sensitive data; sanitize
or redact the stdout before embedding it in the error message (e.g., strip
tokens/credentials, replace long runs with "[REDACTED]" or show only a safe
indicator like "<output suppressed>" and keep waitedFor and exitCode intact).
Update the code that builds the error message to process scope.stdout (use a
sanitizer function or a short fixed placeholder) and ensure both occurrences
that slice scope.stdout are replaced with the sanitized value so logs/tests will
not contain raw potential secrets.
In @src/tests/runShell.js:
- Around line 251-257: The checkStdioPattern function currently constructs and
executes RegExp from test config with no protections; to mitigate ReDoS,
validate and limit input before creating the regex (e.g., enforce a max pattern
length like 200 chars and reject overly complex patterns), run a safety check
using a library such as safe-regex2 on the pattern string, and execute the regex
test in a non-blocking guarded context (wrap the test in a short timeout or run
the RegExp.test call inside a Worker thread with a hard timeout) so a
catastrophic pattern cannot block the event loop; update checkStdioPattern to
perform these validations and fall back to substring checks when the pattern is
rejected or times out.
In @src/tests/terminateScope.js:
- Around line 16-32: The handler terminateScope must validate
step.terminateScope against the step schema and apply defaults before using it:
call the shared validation helper (e.g., validateStepSchema or
validateAndApplyDefaults) with the terminateScopeStepSchema and step to get a
validatedAction object, handle validation failures by returning a FAIL status
with the validation error message, then use validatedAction (not the raw
step.terminateScope) to derive scopeName; keep the existing scopeRegistry checks
and logging but ensure defaults from the schema are applied before any
processing.
In @test/scopes.test.js:
- Around line 836-846: The polling loop that waits for
registry.get("test-multiline") to include "Welcome to Node.js" can hang
indefinitely; update it to use a bounded wait (e.g., Promise.race between the
polling loop and a timeout) so the wait fails after a max duration (same pattern
as the earlier fix around lines 776-786). Ensure the checkReady loop still polls
scope.stdout and resolves when the string is found, but add a timeout that
rejects (or throws) with a clear message if the string isn’t seen within the max
wait time so tests don’t hang forever.
- Around line 776-786: The polling Promise created around checkReady can hang
indefinitely; modify the Promise so it includes a local timeout (e.g., via
setTimeout or Promise.race) that rejects (or resolves with failure) after a
reasonable interval shorter than the Mocha timeout (for example 10–15s), clear
the timeout when the "Welcome to Node.js" string is detected, and ensure
checkReady's setTimeout loop is stopped when timing out or resolving; locate the
Promise and functions checkReady, registry.get("test-repl"), and scope.stdout to
implement the timeout and cleanup.
🧹 Nitpick comments (8)
src/scopes/code.js (1)
8-14: Consider enhancing placeholder documentation.The placeholder implementation is clear, but could benefit from more detailed JSDoc to guide future implementers:
📚 Enhanced JSDoc suggestion
/** * Create a code REPL scope + * + * @param {Object} options - Configuration options for the code scope + * @param {string} options.name - Name of the scope + * @param {string} [options.language] - Programming language (e.g., 'python', 'node') + * @returns {Promise<Object>} Scope object with REPL session * @throws {Error} Always throws - not yet implemented + * @todo Implement REPL-backed code execution sessions */ async function createCodeScope(options) { throw new Error("Code scopes are not yet implemented"); }src/scopes/registry.js (1)
68-85: Consider logging when appending to non-existent scopes.The silent failure when a scope doesn't exist (Lines 70, 82) could hide bugs if callers use incorrect scope names. Consider adding debug logging to aid troubleshooting.
📝 Suggested enhancement
appendStdout(name, data) { const scope = this.scopes.get(name); if (scope) { scope.stdout += data; + } else { + // Optional: log when attempting to append to non-existent scope + console.debug(`Attempted to append stdout to non-existent scope: ${name}`); } }Apply similar change to
appendStderr.src/tests/typeKeys.js (2)
160-209: Consider making settle timing configurable.The function correctly validates scope existence and writability. However, the hard-coded timing values (settleTime=50ms, maxWaitTime=5000ms at Lines 198-199) may not suit all terminal interactions. Consider making these configurable via step parameters or config.
💡 Suggested enhancement
+ // Use step-defined values or defaults + const settleTime = step.type.settleTime || 50; // ms to wait for output to stabilize + const maxWaitTime = step.type.maxWaitTime || 5000; // max total wait time - const settleTime = 50; // ms to wait for output to stabilize - const maxWaitTime = 5000; // max total wait time await waitForOutputSettle(scopeRegistry, scopeName, settleTime, maxWaitTime);
219-262: Consider increasing pollInterval and monitoring stderr.Two minor observations:
Line 221:
pollInterval = 10msis aggressive and may cause unnecessary CPU usage during the settle wait. Consider increasing to 50ms or making it configurable.Lines 234-240: The function only monitors
stdoutchanges. If the terminal writes tostderr, those changes won't reset the settle timer, potentially causing premature resolution.⚡ Suggested improvements
async function waitForOutputSettle(scopeRegistry, scopeName, settleTime, maxWaitTime) { const startTime = Date.now(); - const pollInterval = 10; // ms between checks + const pollInterval = 50; // ms between checks let lastStdoutLength = 0; + let lastStderrLength = 0; let lastChangeTime = Date.now(); return new Promise((resolve) => { const check = () => { const scope = scopeRegistry.get(scopeName); if (!scope) { resolve(); // Scope gone, nothing to wait for return; } const currentLength = scope.stdout.length; + const currentStderrLength = scope.stderr.length; - if (currentLength !== lastStdoutLength) { + if (currentLength !== lastStdoutLength || currentStderrLength !== lastStderrLength) { // Output changed, reset settle timer lastStdoutLength = currentLength; + lastStderrLength = currentStderrLength; lastChangeTime = Date.now(); }src/scopes/terminal.js (2)
74-88: Consider using event-based waiting instead of polling.The polling approach with
setTimeoutat 10ms intervals works but is less efficient than an event-based approach. SinceonExitis already set up on line 69, you could resolve the promise directly from theonExithandler rather than polling.♻️ Suggested refactor using event-driven approach
- if (waitForExit) { - // Wait for process to exit - const checkExit = () => { - if (exited) { - resolve({ - process: ptyProcess, - stdout, - stderr, - exitCode, - }); - } else { - setTimeout(checkExit, 10); - } - }; - checkExit(); + if (waitForExit) { + // Wait for process to exit via onExit event + ptyProcess.onExit(({ exitCode: code }) => { + exited = true; + exitCode = code; + resolve({ + process: ptyProcess, + stdout, + stderr, + exitCode, + }); + });Note: This would require removing the duplicate
onExithandler at line 69 and consolidating the logic.
153-167: Redundant safety timeout at line 166.There are two timeouts: one at line 153 for force-kill after
timeoutms, and another at line 166 for cleanup aftertimeout + 100ms. The second timeout is redundant since the force-kill timeout already callscleanup()at line 162. The only scenario where line 166 would matter is ifprocess.kill("SIGKILL")somehow hangs, which is unlikely.Consider removing line 166 to simplify the logic, or add a comment explaining why the extra 100ms buffer is needed.
src/tests/runShell.js (2)
160-163: Inconsistent cleanup: process not terminated before deleting scope.When the command completes normally (without waitUntil), line 162 calls
scopeRegistry.delete(scopeName)without first terminating the process. While the process has exited at this point (since we're in the exit path), this differs from thecleanupScopehelper which explicitly callsscope.process.kill()before deletion.For consistency and safety (in case exitCode detection has edge cases), consider using
cleanupScopehere as well, or at minimum add a comment explaining why direct deletion is safe.
205-231: Polling-based exit detection duplicates logic from terminal.js.The
waitForExitfunction here uses the same polling pattern ascreateTerminalScopein terminal.js. Consider extracting this into a shared utility to reduce duplication and ensure consistent behavior.As per coding guidelines, prefer DRY principles. This polling logic could be moved to a shared utility in
src/scopes/orsrc/utils.js.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
-
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
dev/dev.spec.jsondev/index.jspackage.jsonsrc/config.jssrc/scopes/cleanup.jssrc/scopes/code.jssrc/scopes/index.jssrc/scopes/registry.jssrc/scopes/terminal.jssrc/scopes/waitUntil.jssrc/tests.jssrc/tests/runCode.jssrc/tests/runShell.jssrc/tests/terminateScope.jssrc/tests/typeKeys.jstest/artifacts/scope_basic.spec.jsontest/artifacts/scope_errors.spec.jsontest/artifacts/scope_lifecycle.spec.jsontest/artifacts/scope_typeKeys.spec.jsontest/artifacts/scope_waitUntil.spec.jsontest/scopes.test.js
🧰 Additional context used
📓 Path-based instructions (5)
src/tests/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/tests/*.js: Create new step type handlers as async functions exported fromsrc/tests/[actionName].js
Step handlers must return object with{ status: "PASS"|"FAIL"|"WARNING", description: string, outputs: {} }
Validate step schema before resolving to object and setting defaults in new step handlers
Always handle driver cleanup in try/finally blocks in browser automation code
Set step variables viastep.variables = { MY_VAR: "$$response.body.token" }to store data as environment variables
UsesetViewportSize()to calculate inner dimensions rather than directly setting window size in browser automation
Mark unsafe steps withstep.unsafe = trueand requireconfig.allowUnsafeSteps = trueto execute
Files:
src/tests/terminateScope.jssrc/tests/runCode.jssrc/tests/runShell.jssrc/tests/typeKeys.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Uselog(config, level, message)for all logging, where level = "debug"|"info"|"warning"|"error"
UsegetAvailableApps()to detect installed browsers instead of hardcoding browser paths
Files:
src/tests/terminateScope.jssrc/scopes/registry.jssrc/scopes/terminal.jssrc/tests/runCode.jssrc/scopes/cleanup.jssrc/config.jssrc/scopes/code.jssrc/tests/runShell.jssrc/scopes/waitUntil.jssrc/tests/typeKeys.jssrc/scopes/index.jssrc/tests.js
src/config.js
📄 CodeRabbit inference engine (AGENTS.md)
src/config.js: Configuration must be validated viadoc-detective-commonschemas usingvalidate({ schemaKey: "config_v3", object })
UseloadEnvs()andreplaceEnvs()for environment variable handling with$VAR_NAMEsyntax
OpenAPI definitions must be loaded and dereferenced at config time and stored inconfig.integrations.openApi[].definition
Browsers and drivers must match the current platform (detected via@puppeteer/browsers) with Appium drivers installed for chromium, gecko, and safari
Files:
src/config.js
test/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
test/**/*.js: Usefs.writeFileSync()+fs.unlinkSync()in try/finally blocks for temporary test files to ensure cleanup
HTTP request tests must run against test server on port 8092 intest/server/
Files:
test/scopes.test.js
src/tests.js
📄 CodeRabbit inference engine (AGENTS.md)
src/tests.js: Add new driver-based step actions to thedriverActionsarray insrc/tests.jsand implement case inrunStep()switch statement
UsestepExecutionFailedflag to skip remaining steps in a context after first failure
Set Appium driver timeout to 10 minutes (newCommandTimeout: 600) for all driver-based steps
runSpecs()insrc/tests.jsorchestrates test execution flow: spec → test → context → step hierarchy
Files:
src/tests.js
🧠 Learnings (23)
📓 Common learnings
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests.js : `runSpecs()` in `src/tests.js` orchestrates test execution flow: spec → test → context → step hierarchy
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Tests follow nested hierarchy: spec (file) → test → context (browser/platform combo) → step (action), with contexts running serially and steps skipping after first failure
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: `src/tests.js` is the core orchestrator handling 600+ LOC of Appium/WebDriver management
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/index.js : `src/index.js` must export `runTests()` as the main entry point for the package
Applied to files:
dev/index.jssrc/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/config.js : Configuration must be validated via `doc-detective-common` schemas using `validate({ schemaKey: "config_v3", object })`
Applied to files:
dev/index.jssrc/tests/typeKeys.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/config.js : Browsers and drivers must match the current platform (detected via `puppeteer/browsers`) with Appium drivers installed for chromium, gecko, and safari
Applied to files:
package.jsonsrc/config.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: `src/tests.js` is the core orchestrator handling 600+ LOC of Appium/WebDriver management
Applied to files:
package.jsonsrc/tests/terminateScope.jstest/artifacts/scope_waitUntil.spec.jsontest/scopes.test.jsdev/dev.spec.jsontest/artifacts/scope_lifecycle.spec.jsontest/artifacts/scope_errors.spec.jsontest/artifacts/scope_basic.spec.jsonsrc/tests.jstest/artifacts/scope_typeKeys.spec.json
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/**/*.js : Use `getAvailableApps()` to detect installed browsers instead of hardcoding browser paths
Applied to files:
package.jsonsrc/config.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Create new step type handlers as async functions exported from `src/tests/[actionName].js`
Applied to files:
src/tests/terminateScope.jstest/artifacts/scope_waitUntil.spec.jsontest/scopes.test.jsdev/dev.spec.jsonsrc/tests/typeKeys.jstest/artifacts/scope_errors.spec.jsonsrc/tests.jstest/artifacts/scope_typeKeys.spec.json
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests.js : `runSpecs()` in `src/tests.js` orchestrates test execution flow: spec → test → context → step hierarchy
Applied to files:
src/tests/terminateScope.jssrc/tests/runCode.jstest/artifacts/scope_waitUntil.spec.jsontest/scopes.test.jssrc/tests/runShell.jsdev/dev.spec.jsontest/artifacts/scope_lifecycle.spec.jsonsrc/scopes/index.jstest/artifacts/scope_errors.spec.jsontest/artifacts/scope_basic.spec.jsonsrc/tests.jstest/artifacts/scope_typeKeys.spec.json
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Step handlers must return object with `{ status: "PASS"|"FAIL"|"WARNING", description: string, outputs: {} }`
Applied to files:
src/tests/terminateScope.jstest/artifacts/scope_waitUntil.spec.jsontest/scopes.test.jsdev/dev.spec.jsontest/artifacts/scope_lifecycle.spec.jsontest/artifacts/scope_errors.spec.jsontest/artifacts/scope_basic.spec.jsonsrc/tests.jstest/artifacts/scope_typeKeys.spec.json
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests.js : Use `stepExecutionFailed` flag to skip remaining steps in a context after first failure
Applied to files:
src/tests/terminateScope.jstest/scopes.test.jstest/artifacts/scope_lifecycle.spec.jsontest/artifacts/scope_errors.spec.jsontest/artifacts/scope_basic.spec.jsonsrc/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests.js : Add new driver-based step actions to the `driverActions` array in `src/tests.js` and implement case in `runStep()` switch statement
Applied to files:
src/tests/terminateScope.jssrc/tests/runCode.jssrc/tests/runShell.jsdev/dev.spec.jsonsrc/tests/typeKeys.jssrc/tests.jstest/artifacts/scope_typeKeys.spec.json
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Mark unsafe steps with `step.unsafe = true` and require `config.allowUnsafeSteps = true` to execute
Applied to files:
src/tests/terminateScope.jstest/scopes.test.jstest/artifacts/scope_lifecycle.spec.jsontest/artifacts/scope_errors.spec.jsontest/artifacts/scope_basic.spec.jsonsrc/tests.jstest/artifacts/scope_typeKeys.spec.json
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to test/**/*.js : Use `fs.writeFileSync()` + `fs.unlinkSync()` in try/finally blocks for temporary test files to ensure cleanup
Applied to files:
src/tests/terminateScope.jstest/scopes.test.jssrc/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Validate step schema before resolving to object and setting defaults in new step handlers
Applied to files:
src/tests/terminateScope.jssrc/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to test/core.test.js : Test suite runs with mocha using `this.timeout(0)` for indefinite timeout in `test/core.test.js`
Applied to files:
test/artifacts/scope_waitUntil.spec.jsontest/scopes.test.jstest/artifacts/scope_lifecycle.spec.jsontest/artifacts/scope_errors.spec.jsontest/artifacts/scope_basic.spec.jsonsrc/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Individual step types have dedicated handler implementations in `src/tests/` (e.g., `httpRequest.js`, `runShell.js`)
Applied to files:
test/artifacts/scope_waitUntil.spec.jsontest/scopes.test.jsdev/dev.spec.jsontest/artifacts/scope_lifecycle.spec.jsontest/artifacts/scope_errors.spec.jsontest/artifacts/scope_basic.spec.jsonsrc/tests.jstest/artifacts/scope_typeKeys.spec.json
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Tests follow nested hierarchy: spec (file) → test → context (browser/platform combo) → step (action), with contexts running serially and steps skipping after first failure
Applied to files:
test/artifacts/scope_waitUntil.spec.jsontest/scopes.test.jsdev/dev.spec.jsontest/artifacts/scope_lifecycle.spec.jsontest/artifacts/scope_errors.spec.jsontest/artifacts/scope_basic.spec.jsonsrc/tests.jstest/artifacts/scope_typeKeys.spec.json
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests.js : Set Appium driver timeout to 10 minutes (`newCommandTimeout: 600`) for all driver-based steps
Applied to files:
test/artifacts/scope_waitUntil.spec.jsontest/scopes.test.jsdev/dev.spec.jsontest/artifacts/scope_basic.spec.jsonsrc/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to test/**/*.js : HTTP request tests must run against test server on port 8092 in `test/server/`
Applied to files:
test/artifacts/scope_waitUntil.spec.json
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Set step variables via `step.variables = { MY_VAR: "$$response.body.token" }` to store data as environment variables
Applied to files:
test/artifacts/scope_waitUntil.spec.jsondev/dev.spec.jsontest/artifacts/scope_lifecycle.spec.jsontest/artifacts/scope_errors.spec.jsontest/artifacts/scope_basic.spec.jsonsrc/tests.jstest/artifacts/scope_typeKeys.spec.json
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to scripts/postinstall.js : Post-install script must auto-download Chrome/Firefox/ChromeDriver to `browser-snapshots/` directory
Applied to files:
src/config.js
📚 Learning: 2025-08-22T22:04:55.486Z
Learnt from: hawkeyexl
Repo: doc-detective/core PR: 349
File: src/tests/dragAndDrop.js:76-85
Timestamp: 2025-08-22T22:04:55.486Z
Learning: In the doc-detective codebase, action functions like dragAndDropElement are only called when the corresponding action property (e.g., step.dragAndDrop) is present in the step, eliminating the need for defensive checks for the action configuration. Schema validation including required fields is handled by ajv through the validate function from doc-detective-common.
Applied to files:
src/tests/typeKeys.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Always handle driver cleanup in try/finally blocks in browser automation code
Applied to files:
src/tests.js
🧬 Code graph analysis (9)
src/scopes/registry.js (4)
test/scopes.test.js (8)
scope(55-55)scope(65-65)scope(121-121)scope(132-132)scope(146-146)scope(159-159)scope(802-802)scope(859-859)src/tests/typeKeys.js (1)
scope(179-179)src/tests/runShell.js (3)
scope(134-134)scope(156-156)scope(238-238)src/tests/terminateScope.js (1)
scope(44-44)
src/scopes/terminal.js (1)
src/scopes/index.js (3)
require(8-8)require(9-9)require(10-10)
src/tests/runCode.js (1)
test/scopes.test.js (6)
step(687-691)step(717-721)step(788-793)step(849-854)step(2006-2006)step(2046-2046)
src/scopes/cleanup.js (3)
src/tests/typeKeys.js (1)
scope(179-179)src/tests/runShell.js (3)
scope(134-134)scope(156-156)scope(238-238)src/tests/terminateScope.js (1)
scope(44-44)
src/config.js (1)
src/tests.js (1)
chrome(1033-1035)
test/scopes.test.js (5)
src/scopes/index.js (5)
require(8-8)require(9-9)require(10-10)require(11-11)require(12-12)src/tests/terminateScope.js (1)
scope(44-44)src/utils.js (1)
data(75-75)src/scopes/terminal.js (1)
process(24-32)src/scopes/waitUntil.js (1)
startTime(72-72)
src/scopes/waitUntil.js (2)
src/tests/terminateScope.js (2)
scopeName(21-21)scope(44-44)src/utils.js (1)
exitCode(254-256)
src/tests/typeKeys.js (1)
src/scopes/waitUntil.js (2)
startTime(72-72)pollInterval(73-73)
src/tests.js (1)
test/scopes.test.js (7)
ScopeRegistry(16-16)step(687-691)step(717-721)step(788-793)step(849-854)step(2006-2006)step(2046-2046)
🔇 Additional comments (40)
src/config.js (1)
319-338: Good hardening: avoid crash when Chrome isn't present (chrome?.buildId).The optional chaining prevents a
TypeErrorwhen Chrome isn't found. The fieldsbuildIdandexecutablePathare correctly retrieved from@puppeteer/browsers@2.11.0'sgetInstalledBrowsers()API, so the version assignment and driver path are properly sourced.dev/index.js (2)
10-12: LGTM: Dev config updated for local testing.The switch from Heretto-based input to a local test spec file aligns with the PR objectives to test the new scope-based functionality.
29-37: LGTM: Heretto integration preserved as reference.Commenting out the Heretto integration rather than removing it preserves useful example configuration for future reference.
src/scopes/cleanup.js (2)
28-36: Consider library usage: process.exit() may terminate host application.The handlers unconditionally call
process.exit(), which will terminate any application using this package as a library. Consider whether this is the desired behavior or if the cleanup should be non-terminating when used in embedded contexts.Do you want the cleanup handlers to always terminate the process, or should there be an option to perform cleanup without exiting (for library consumers)?
Also applies to: 60-70
38-58: Registry access is correct and uses public API methods.The methods
registry.list()(line 91) andregistry.get()(line 30) are documented public API methods ofScopeRegistry, andscope.processis a legitimate public property. The exit handler appropriately uses these public APIs for synchronous cleanup, which is necessary since the "exit" event handler cannot be asynchronous.package.json (2)
39-40: Verify local dependencies are intentional for this branch/workflow.The dependencies for
doc-detective-commonanddoc-detective-resolverhave been changed to local file references (file:../common,file:../resolver). Confirm this is intentional for the development workflow on this branch and that there's a process to restore registry versions before publishing.Are these local dependencies:
- Intentional for monorepo development?
- Will they be reverted to version pins before merging/publishing?
44-44: node-pty 1.1.0 uses prebuilt binaries—no native build toolchain needed.The version specified (@lydell/node-pty 1.1.0 fork) ships with prebuilt platform-specific binaries for macOS (x64, ARM64), Linux (x64, ARM64), and Windows (x64, ARM64), eliminating the need for local compilation during installation. The CI/CD pipelines already validate compatibility across platforms (ubuntu-latest, windows-latest, macos-latest) and test against Node.js versions 20, 22, and 24—all compatible with the recommended Node.js 16+ requirement.
dev/dev.spec.json (3)
6-14: LGTM: Scope-based Node REPL test implemented correctly.The test properly:
- Uses
nodecommand as specified in PR objectives- Creates a named scope (
basic-scope)- Waits for the Node.js welcome message using
waitUntilpattern- Demonstrates the new scope-based terminal interaction functionality
17-21: LGTM: Validates typed input capture in interactive terminal.This step correctly tests that:
- Keys can be typed into a scoped terminal
- Interactive REPL commands are captured
- Special keys like
$ENTER$are handled- Multi-step interactions work within a scope
26-28: LGTM: Proper scope cleanup.The
terminateScopestep ensures the named scope is properly terminated, completing the lifecycle test.test/artifacts/scope_waitUntil.spec.json (1)
1-136: Well-structured waitUntil test scenarios.The test artifact comprehensively covers various waitUntil patterns including string matching, regex patterns, case-insensitive matching, multiline output, fast/immediate output, and delayed output. The use of
shwithechoandsleepcommands is appropriate for validating stdout pattern matching behavior.src/tests/runCode.js (3)
40-40: LGTM: Function signature correctly updated.The function signature properly accepts
scopeRegistryparameter, enabling scoped code execution support.
116-123: LGTM: Scope and waitUntil forwarding.The code correctly forwards
scopeandwaitUntilproperties fromstep.runCodeto the underlyingshellStep.runShell, enabling scoped execution and output synchronization for code execution steps.
128-128: LGTM: scopeRegistry propagation.The
scopeRegistryis correctly passed torunShell, enabling the scope management system to function through the code execution path.src/tests.js (7)
23-23: LGTM: terminateScope import.The terminateScope action handler is correctly imported for integration into the step execution flow.
31-31: LGTM: ScopeRegistry import.The ScopeRegistry is correctly imported from the scopes module to enable per-context scope management.
524-525: LGTM: Per-context scope registry.Creating a new
ScopeRegistryinstance per context ensures proper isolation of scoped resources between different test contexts, preventing cross-contamination of scopes.
654-654: LGTM: scopeRegistry propagation to runStep.The
scopeRegistryis correctly passed torunStep, enabling scope-aware actions throughout the step execution hierarchy.
751-761: LGTM: Scope cleanup with error handling.The cleanup logic properly handles scope termination after context completion with appropriate error logging. The placement after driver cleanup ensures browser resources are released before terminal scopes.
852-852: LGTM: runStep signature enhancement.The
runStepfunction signature correctly acceptsscopeRegistryparameter for propagation to scope-aware action handlers.
907-923: LGTM: scopeRegistry distribution to action handlers.The
scopeRegistryis correctly passed to all scope-aware action handlers (runCode,runShell,terminateScope,typeKeys), enabling the scope management system throughout the execution flow.src/tests/terminateScope.js (2)
43-63: LGTM: Scope termination logic.The implementation correctly terminates the scope process, removes it from the registry, and returns appropriate outputs including captured stdout/stderr. Error handling ensures graceful failure with descriptive messages.
56-63: No action needed—stdout/stderr are guaranteed to exist.The scope registry's
create()method always initializesstdoutandstderras empty strings (lines 47-52 in registry.js). Since scopes are only created and stored through this method, every scope retrieved will have these properties. The fallback to empty strings in terminateScope.js is redundant but harmless.Likely an incorrect or invalid review comment.
test/artifacts/scope_typeKeys.spec.json (1)
1-165: This file is not part of the PR scope.The PR objectives specifically state "Change shell command from 'sh' to 'node' in dev.spec.json," and dev.spec.json has already been updated to use
"command": "node"for Node REPL testing. The scope_typeKeys.spec.json file is a generic test artifact for shell interaction functionality (typing, special keys, tabs, REPL-like behavior, and Ctrl+C handling) and was not modified in this PR. The presence of a "repl-scope" test does not indicate it should use Node REPL—it simulates REPL behavior using standard shell input/output patterns that are distinct from the actual Node REPL testing implemented in dev.spec.json.Likely an incorrect or invalid review comment.
src/tests/typeKeys.js (4)
70-103: LGTM - Standard terminal escape sequences.The terminal special key mappings use standard VT100/ANSI escape sequences, which are widely compatible with modern terminals and PTY implementations.
106-106: LGTM - Clean routing between scope and browser typing.The updated function signature and routing logic appropriately delegates to scope-based or browser-based typing depending on the presence of a scope name.
Also applies to: 145-155
269-312: LGTM - Robust key conversion with proper fallback handling.The key conversion functions correctly handle both standalone special keys and embedded special keys within strings. The fallback to literal characters (Lines 307-308) ensures that strings with "$" characters that don't match special key patterns are handled gracefully.
314-404: LGTM - Clean refactoring of browser typing logic.The extraction of browser-based typing into a separate function improves code organization without altering the existing logic.
src/scopes/terminal.js (2)
1-10: LGTM!Clean module documentation and appropriate imports for PTY-backed terminal functionality.
134-150: Potential race condition:onExitlistener registered afterkill()is called.The
onExitlistener is registered at line 135-138, butprocess.kill()is called immediately after at line 144. If the process exits synchronously or very quickly after the kill signal, theonExitevent might fire before the listener is attached, causing the promise to rely solely on the force-kill timeout for resolution.🔧 Suggested fix: Register onExit before calling kill
+ // Listen for exit first (before sending kill signal) + if (process.onExit) { + process.onExit(() => { + cleanup(); + }); + } + // Try graceful termination first (SIGTERM) try { if (typeof process.kill === "function") { process.kill(); } } catch (e) { // Process may already be dead cleanup(); return; } - - // Listen for exit - if (process.onExit) { - process.onExit(() => { - cleanup(); - }); - }Likely an incorrect or invalid review comment.
src/tests/runShell.js (2)
7-10: LGTM!Appropriate imports for the new scoped command functionality.
73-196: Well-structured scoped command implementation.The
runScopedCommandfunction cleanly handles both waitUntil (background process) and non-waitUntil (foreground process) modes with proper cleanup in all code paths. Good use of try/catch with cleanup on error.src/scopes/index.js (1)
1-32: LGTM!Clean barrel file implementation with well-organized exports grouped by functionality. This provides a cohesive public API for the scopes module.
src/scopes/waitUntil.js (3)
41-46: LGTM!Clean and simple pattern matching implementation with proper handling of both regex and string patterns.
60-135: Well-designed condition waiting with comprehensive error handling.The
waitForConditionsfunction properly handles:
- Pattern parsing upfront to fail fast on invalid regex
- Both stdout and stderr conditions
- Early process exit detection
- Timeout with descriptive error messages
- Polling with reasonable interval
13-33: The regex flag validation is correct and properly supports ES2024+ features.The
parsePatternfunction correctly validates all valid regex flags including the 'v' flag (unicodeSets), which requires ES2024 support. Node.js 20.10.0+ supports this flag, and the CI pipeline tests on Node.js 20, 22, and 24, all of which include the necessary support. The function properly catches and reports errors for invalid regex patterns via try-catch at lines 19–24.Note: As mentioned in the original concern, user-supplied regex patterns could still pose a ReDoS risk when evaluated in
matchesPatternregardless of flag validation.test/scopes.test.js (4)
18-26: Good use of graceful degradation with try/catch for module imports.The pattern of wrapping
require()in try/catch withthis.skip()allows tests to gracefully skip when modules don't exist yet (RED phase of TDD). This is a thoughtful approach for test-driven development.Also applies to: 237-245, 436-445, 621-629, 668-674, 741-750
922-936: Excellent adherence to temp file cleanup guidelines.The integration tests properly use
fs.writeFileSync()+fs.unlinkSync()in try/finally blocks for temporary test files, ensuring cleanup even when tests fail. This follows the coding guidelines exactly.Also applies to: 970-983
1070-1125: Comprehensive typeKeys integration tests with proper Windows skipping.Good coverage of:
- Sending keys to named scopes
- Failure handling for non-existent scopes
- Special key translation (
$ENTER$ ,$TAB$ )- Auto-settle behavior verification
- Node REPL interaction
All platform-specific tests correctly skip on Windows.
Also applies to: 1159-1212, 1214-1278, 1280-1357
2056-2407: Excellent coverage of scope execution behavior edge cases.The "Scope execution behavior" describe block thoroughly tests the nuanced behavior of:
- waitUntil present vs absent
- Timeout handling in both modes
- Exit code validation
- Early process exit detection
- Background vs foreground process semantics
This comprehensive test coverage will help prevent regressions.
| const uncaughtHandler = async (err) => { | ||
| console.error("Uncaught exception:", err); | ||
| await cleanup(); | ||
| process.exit(1); | ||
| }; |
There was a problem hiding this comment.
Use the framework's logging mechanism instead of console.error.
According to the coding guidelines, all logging in src/**/*.js should use log(config, level, message) instead of console.error.
📝 Recommended approach
Since cleanup handlers need to be usable without a config object, consider:
- Accept an optional config parameter in
setupCleanupHandlers(registry, config) - Use conditional logging:
if (config) log(config, "error", message)or fall back to console.error when config is unavailable - Document that config is optional but recommended for proper logging integration
Based on coding guidelines.
Also applies to: 66-70
🤖 Prompt for AI Agents
In @src/scopes/cleanup.js around lines 60 - 64, The uncaught exception handler
uses console.error; change setupCleanupHandlers to accept an optional config
parameter (setupCleanupHandlers(registry, config)) and have uncaughtHandler (and
the other handlers at 66-70) use the framework logger: if (config) log(config,
"error", "Uncaught exception: " + String(err)) else console.error("Uncaught
exception:", err); ensure the handlers close over the optional config, still
call await cleanup() and process.exit(1), and update any similar places (lines
66-70) to follow the same conditional logging pattern.
| async cleanup() { | ||
| const promises = []; | ||
| for (const [name, scope] of this.scopes) { | ||
| if (scope.process && typeof scope.process.kill === "function") { | ||
| try { | ||
| scope.process.kill(); | ||
| } catch (e) { | ||
| // Ignore errors during cleanup | ||
| } | ||
| } | ||
| promises.push(Promise.resolve()); | ||
| } | ||
| await Promise.all(promises); | ||
| this.scopes.clear(); | ||
| } |
There was a problem hiding this comment.
Fix cleanup method to properly terminate processes and wait for completion.
The cleanup implementation has two issues:
-
Line 104:
kill()without a signal argument may not terminate processes gracefully. Consider usingSIGTERMfirst with a fallback toSIGKILL. -
Lines 109-111: The promise handling is ineffective -
Promise.resolve()immediately resolves, soPromise.all(promises)doesn't actually wait for process termination. This could lead to incomplete cleanup or orphaned processes.
🔧 Proposed fix for graceful termination
async cleanup() {
const promises = [];
for (const [name, scope] of this.scopes) {
if (scope.process && typeof scope.process.kill === "function") {
- try {
- scope.process.kill();
- } catch (e) {
- // Ignore errors during cleanup
- }
+ promises.push(
+ new Promise((resolve) => {
+ try {
+ // Try graceful termination first
+ scope.process.kill("SIGTERM");
+
+ // Set timeout for forceful termination
+ const timeout = setTimeout(() => {
+ try {
+ scope.process.kill("SIGKILL");
+ } catch (e) {
+ // Process already terminated
+ }
+ resolve();
+ }, 2000); // 2 second grace period
+
+ // Listen for process exit
+ scope.process.once("exit", () => {
+ clearTimeout(timeout);
+ resolve();
+ });
+ } catch (e) {
+ // Process already terminated or error
+ resolve();
+ }
+ })
+ );
+ } else {
+ promises.push(Promise.resolve());
}
- promises.push(Promise.resolve());
}
await Promise.all(promises);
this.scopes.clear();
}🤖 Prompt for AI Agents
In @src/scopes/registry.js around lines 99 - 113, The cleanup method currently
calls scope.process.kill() without a signal and pushes Promise.resolve(), so it
neither requests graceful termination nor waits for exit; update the cleanup
(method cleanup on the registry class that iterates this.scopes) to first
attempt scope.process.kill('SIGTERM') inside a try/catch, create and push a
Promise that resolves when the child process emits 'exit' or 'close' (and
rejects or times out after a configurable timeout, e.g. 5s), and if the timeout
elapses call scope.process.kill('SIGKILL') and wait for final exit; ensure you
remove event listeners and handle synchronous kill errors to avoid unhandled
exceptions, then await Promise.all(promises) before clearing this.scopes.
| reject(new Error( | ||
| `Process exited with code ${exitCode} before condition was met (${waitedFor.join(", ")}). ` + | ||
| `Output: "${scope.stdout.slice(-200)}"` | ||
| )); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Sensitive data exposure risk in error messages.
Error messages at lines 111 and 124 include the last 200 characters of stdout, which could contain sensitive information (credentials, tokens, etc.) if the process outputs such data. These error messages may end up in logs or test reports.
Consider truncating differently or sanitizing the output before including it in error messages.
🔧 Suggested improvement
- reject(new Error(
- `Process exited with code ${exitCode} before condition was met (${waitedFor.join(", ")}). ` +
- `Output: "${scope.stdout.slice(-200)}"`
- ));
+ const truncatedOutput = scope.stdout.slice(-200).replace(/[\r\n]+/g, ' ');
+ reject(new Error(
+ `Process exited with code ${exitCode} before condition was met (${waitedFor.join(", ")}). ` +
+ `Output (last 200 chars): "${truncatedOutput}"`
+ ));Also applies to: 122-126
🤖 Prompt for AI Agents
In @src/scopes/waitUntil.js around lines 109 - 114, The error construction that
includes raw scope.stdout (the reject(new Error(...)) in waitUntil.js) can leak
sensitive data; sanitize or redact the stdout before embedding it in the error
message (e.g., strip tokens/credentials, replace long runs with "[REDACTED]" or
show only a safe indicator like "<output suppressed>" and keep waitedFor and
exitCode intact). Update the code that builds the error message to process
scope.stdout (use a sanitizer function or a short fixed placeholder) and ensure
both occurrences that slice scope.stdout are replaced with the sanitized value
so logs/tests will not contain raw potential secrets.
| function checkStdioPattern(pattern, stdout, stderr) { | ||
| if (pattern.startsWith("/") && pattern.endsWith("/")) { | ||
| const regex = new RegExp(pattern.slice(1, -1)); | ||
| return regex.test(stdout) || regex.test(stderr); | ||
| } | ||
| return stdout.includes(pattern) || stderr.includes(pattern); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the relevant file to understand context
cat -n src/tests/runShell.js | head -300 | tail -80Repository: doc-detective/core
Length of output: 2876
🏁 Script executed:
# Search for calls to checkStdioPattern to understand the input source
rg "checkStdioPattern" src/tests/runShell.js -B 5 -A 2Repository: doc-detective/core
Length of output: 630
🌐 Web query:
ReDoS prevention best practices Node.js regex security
💡 Result:
Concise ReDoS prevention best practices for Node.js (actionable):
-
Avoid vulnerable patterns: no nested quantifiers or overlapping alternation (e.g., (a+)+, (a|aa)+). Review hard-coded regexes for catastrophic backtracking. [2][4]
-
Prefer a non-backtracking engine for untrusted regexes: use RE2/node-re2 to eliminate exponential backtracking (trade-offs: no backreferences/lookahead). Use RE2 when users supply regexes. [1][2]
-
Static-check regexes before use: run safe-regex / safe-regex2 or vuln-regex-detector in CI or at runtime to flag risky patterns (note: tools have false positives/negatives). [3][2]
-
Limit and validate input size: enforce strict length limits and input character whitelists before running heavy regexes. [2][5]
-
Timeouts / isolation: run expensive matches in a separate worker (Worker Threads or child process) and enforce a timeout or kill the worker if it exceeds allowed CPU/time. That prevents blocking the main Node.js event loop. [2][5]
-
Use anchored/targeted patterns and character classes: prefer explicit classes (e.g., [^b]* vs .*), anchors (^,$), and possessive/atomic constructs if supported to eliminate backtracking. [2]
-
Prefer parsing over complex regexes: when logic gets complex, use a parser or deterministic algorithm instead of a single giant regex. [2][4]
-
Sanitize third‑party dependencies: scan deps for disclosed ReDoS advisories and update/replace vulnerable packages. [5][7]
Quick implementation checklist:
- Replace user-supplied RegExp with node-re2 or reject unsupported features. [1]
- Run safe-regex2 in CI for all regex literals. [3]
- Enforce input length limits + run regexes in Worker/child process with timeout. [2][5]
- Audit dependencies for ReDoS advisories. [5]
Sources: RE2/node-re2 docs [1]; regular-expressions.info ReDoS guidance [2]; safe-regex2 (npm) [3]; OWASP Node.js security cheat sheet [4]; Snyk analysis on ReDoS in npm [5].
Add safeguards against ReDoS in regex pattern matching.
The checkStdioPattern function accepts user-supplied regex patterns from test configuration without validation or timeout protection (line 253). While the input comes from developer test files rather than untrusted external sources, a malicious or poorly-crafted regex could still cause catastrophic backtracking and block the Node.js event loop.
Apply one or more of these practical mitigations:
- Wrap the regex test in a timeout or Worker thread to prevent blocking
- Validate regex patterns against known risky constructs before execution (using a library like
safe-regex2) - Add a length limit to the pattern input
🤖 Prompt for AI Agents
In @src/tests/runShell.js around lines 251 - 257, The checkStdioPattern function
currently constructs and executes RegExp from test config with no protections;
to mitigate ReDoS, validate and limit input before creating the regex (e.g.,
enforce a max pattern length like 200 chars and reject overly complex patterns),
run a safety check using a library such as safe-regex2 on the pattern string,
and execute the regex test in a non-blocking guarded context (wrap the test in a
short timeout or run the RegExp.test call inside a Worker thread with a hard
timeout) so a catastrophic pattern cannot block the event loop; update
checkStdioPattern to perform these validations and fall back to substring checks
when the pattern is rejected or times out.
| async function terminateScope(config, step, context = {}) { | ||
| const { scopeRegistry } = context; | ||
| const action = step.terminateScope; | ||
|
|
||
| // Handle both string and object forms | ||
| const scopeName = typeof action === "string" ? action : action.scope; | ||
|
|
||
| log(config, "debug", `Terminating scope: ${scopeName}`); | ||
|
|
||
| // Validate scope registry | ||
| if (!scopeRegistry) { | ||
| return { | ||
| status: "FAIL", | ||
| description: "Scope registry not available. This action requires scope support.", | ||
| outputs: {}, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add step schema validation.
According to coding guidelines, step handlers must validate the step schema before processing. This handler should validate step.terminateScope against the schema and apply any defaults before executing the action.
As per coding guidelines: "Validate step schema before resolving to object and setting defaults in new step handlers"
🔧 Proposed fix to add schema validation
+const { validate } = require("doc-detective-common");
const { terminateScope: terminateScopeProcess } = require("../scopes");
const { log } = require("../utils");
async function terminateScope(config, step, context = {}) {
+ // Validate step object
+ const isValidStep = validate({ schemaKey: "step_v3", object: step });
+ if (!isValidStep.valid) {
+ return {
+ status: "FAIL",
+ description: `Invalid step definition: ${isValidStep.errors}`,
+ outputs: {},
+ };
+ }
+ // Accept coerced and defaulted values
+ step = isValidStep.object;
+
const { scopeRegistry } = context;
const action = step.terminateScope;🤖 Prompt for AI Agents
In @src/tests/terminateScope.js around lines 16 - 32, The handler terminateScope
must validate step.terminateScope against the step schema and apply defaults
before using it: call the shared validation helper (e.g., validateStepSchema or
validateAndApplyDefaults) with the terminateScopeStepSchema and step to get a
validatedAction object, handle validation failures by returning a FAIL status
with the validation error message, then use validatedAction (not the raw
step.terminateScope) to derive scopeName; keep the existing scopeRegistry checks
and logging but ensure defaults from the schema are applied before any
processing.
| await new Promise((resolve) => { | ||
| const checkReady = () => { | ||
| const scope = registry.get("test-repl"); | ||
| if (scope.stdout.includes("Welcome to Node.js")) { | ||
| resolve(); | ||
| } else { | ||
| setTimeout(checkReady, 50); | ||
| } | ||
| }; | ||
| checkReady(); | ||
| }); |
There was a problem hiding this comment.
Missing timeout in polling loop could cause test to hang.
The checkReady polling loop at lines 776-786 lacks a timeout mechanism. If the Node REPL never outputs "Welcome to Node.js" (e.g., due to environment issues), the test will hang indefinitely despite the 30-second Mocha timeout at line 737.
🔧 Add timeout to prevent indefinite hang
// Wait for Node REPL to be ready
- await new Promise((resolve) => {
+ await new Promise((resolve, reject) => {
+ const startTime = Date.now();
+ const timeout = 10000; // 10 second timeout
const checkReady = () => {
const scope = registry.get("test-repl");
if (scope.stdout.includes("Welcome to Node.js")) {
resolve();
+ } else if (Date.now() - startTime > timeout) {
+ reject(new Error("Timeout waiting for Node REPL to start"));
} else {
setTimeout(checkReady, 50);
}
};
checkReady();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await new Promise((resolve) => { | |
| const checkReady = () => { | |
| const scope = registry.get("test-repl"); | |
| if (scope.stdout.includes("Welcome to Node.js")) { | |
| resolve(); | |
| } else { | |
| setTimeout(checkReady, 50); | |
| } | |
| }; | |
| checkReady(); | |
| }); | |
| await new Promise((resolve, reject) => { | |
| const startTime = Date.now(); | |
| const timeout = 10000; // 10 second timeout | |
| const checkReady = () => { | |
| const scope = registry.get("test-repl"); | |
| if (scope.stdout.includes("Welcome to Node.js")) { | |
| resolve(); | |
| } else if (Date.now() - startTime > timeout) { | |
| reject(new Error("Timeout waiting for Node REPL to start")); | |
| } else { | |
| setTimeout(checkReady, 50); | |
| } | |
| }; | |
| checkReady(); | |
| }); |
🤖 Prompt for AI Agents
In @test/scopes.test.js around lines 776 - 786, The polling Promise created
around checkReady can hang indefinitely; modify the Promise so it includes a
local timeout (e.g., via setTimeout or Promise.race) that rejects (or resolves
with failure) after a reasonable interval shorter than the Mocha timeout (for
example 10–15s), clear the timeout when the "Welcome to Node.js" string is
detected, and ensure checkReady's setTimeout loop is stopped when timing out or
resolving; locate the Promise and functions checkReady,
registry.get("test-repl"), and scope.stdout to implement the timeout and
cleanup.
| await new Promise((resolve) => { | ||
| const checkReady = () => { | ||
| const scope = registry.get("test-multiline"); | ||
| if (scope.stdout.includes("Welcome to Node.js")) { | ||
| resolve(); | ||
| } else { | ||
| setTimeout(checkReady, 50); | ||
| } | ||
| }; | ||
| checkReady(); | ||
| }); |
There was a problem hiding this comment.
Same timeout issue in second polling loop.
This polling loop at lines 836-846 has the same issue as the one at lines 776-786 - it lacks a timeout and could hang indefinitely.
Apply the same fix as suggested for lines 776-786.
🤖 Prompt for AI Agents
In @test/scopes.test.js around lines 836 - 846, The polling loop that waits for
registry.get("test-multiline") to include "Welcome to Node.js" can hang
indefinitely; update it to use a bounded wait (e.g., Promise.race between the
polling loop and a timeout) so the wait fails after a max duration (same pattern
as the earlier fix around lines 776-786). Ensure the checkReady loop still polls
scope.stdout and resolves when the string is found, but add a timeout that
rejects (or throws) with a clear message if the string isn’t seen within the max
wait time so tests don’t hang forever.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.